Reworked the sub-type relationship infrastructure #58
Conversation
…ns/operators, more test cases for assignability
… order to reuse and to customize them
… in the type graph, exploit them for advanced assignability checks (WIP)
…ced assignability result+success(+problem)
There was a problem hiding this comment.
Thanks for this work @JohannesMeierSE. I am pretty happy to get rid of analyzeIsSubTypeOf and analyzeIsSuperTypeOf. The rest looks fine to me too, where I had something to add, I placed a comment. Let's discuss the first two "aspects for review" in the meeting. I do not have a ready made solution at hand.
| } | ||
| matchingOverloads.splice(index); // keep only the matches with the same highest priority | ||
| // TODO review: should we make this implementation more efficient? | ||
|
|
There was a problem hiding this comment.
If I understand correctly you are sorting the overloads and then iterate until priority drops (place where prio changes) to then cut the rest, right? I think it is OK, though it might be worth considering to use another data structure in the first place, in the way of a priority queue. Not sure it is worth it, though.
There was a problem hiding this comment.
Maybe do the compare right away and if you get something better, drop everything you have so far and keep the new one instead, if equal, add, if less, drop the candidate, to avoid collecting all of them and having to sort. But in the end, how many candidates do we expect?
There was a problem hiding this comment.
You are completely right, I used the algorithm you sketched and it is much easier than I thought. Now we have linear effort instead of square effort, thanks!
how many candidates do we expect?
The operator/function with the most overloaded signatures is usually "add" (+), which is applied to strings and numbers (and maybe one or two more special types). How many number types might be used in a DSL? Maybe 10, but probably not more than 20 ...
| }); | ||
|
|
||
| test('integer to string', () => { | ||
| expectAssignmentValid(new IntegerLiteral(123), new StringLiteral('456'), 'SubTypeEdge', 'ConversionEdge'); |
There was a problem hiding this comment.
A bit surprising, had to check back with the definitions above :). But OK for test purposes.
There was a problem hiding this comment.
Yes, I agree. We could integrate "language types" into our predefined test nodes, but than we have another file some types in it and I wasn't sure, whether that is a good idea. But it would improve the readability of our test cases. What do you think?
There was a problem hiding this comment.
Might be good to discuss together with @Lotes tomorrow. I think it might be worth the effort.
There was a problem hiding this comment.
@Lotes and @JohannesMeierSE had no convincing idea for an easy solution today.
| typir.Conversion.markAsConvertible(integerType, doubleType, 'IMPLICIT_EXPLICIT'); | ||
| expect(() => typir.Conversion.markAsConvertible(doubleType, integerType, 'IMPLICIT_EXPLICIT')) | ||
| .toThrowError('Adding the conversion from double to integer with mode IMPLICIT_EXPLICIT has introduced a cycle in the type graph.'); | ||
| }); |
There was a problem hiding this comment.
Yep, this was what I'd been thinking of. Thanks!
There was a problem hiding this comment.
If you like, feel free to contribute more test cases like this in a separate PR. I didn't investigated in this PR, how many test cases we need here 🙂
…ld use loops to define multiple relationships at once
…onsTypeInferenceRule, improved comments
|
Thanks @insafuhrmann and @Lotes for your helpful reviews! I just pushed the corresponding improvements (and resolved the corresponding discussions here), but some discussions are still open. I suggest to continue them in our next joint meeting. |
|
@Lotes @insafuhrmann I just pushed the last improvements according to our discussions today. |
This PR contributes:
analyzeIsSubTypeOfandanalyzeIsSuperTypeOfanymoreSubTypeProblemandSubTypeSuccessinterfaces, combined astype SubTypeResult = SubTypeSuccess | SubTypeProblemtypirinpackages/typir/src/test/predefined-language-nodes.ts(they are strongly inspired from our joint work @insafuhrmann)Aspects for the review:
subProblemsfor failed checks of sub-type and assignability? As an example, a user is interested, why two structually typed classes are not sub-types.GraphAlgorithmsingraph-algorithms.tsmore type-safe?conversion.test.tscontains a test case to check for cycles in conversion rules: Could you check, whether is reflects the kind of test cases you looked for you?